Skip to content
This repository has been archived by the owner on Feb 2, 2023. It is now read-only.

feat(is-routing): support instantsearch routing #551

Merged
merged 5 commits into from
Aug 8, 2018

Conversation

JonathanMontane
Copy link
Contributor

@JonathanMontane JonathanMontane commented Aug 3, 2018

Summary
This PR makes Places InstantSearch Widget support the routing API from InstantSearch.

Result
The Places widget now exposes the following object to the uiState:

{
  places: {
     position: '123,123',
     query: 'Paris'
  }
}
  • position is the variable used to update the aroundLatLng
  • query is the value of the input when a places record has been selected

Demo
https://places-routing.surge.sh

makes Places InstantSearch Widget support the routing API
from InstantSearch.
const getInitializedWidget = () => {
const client = createFakeClient();
const helper = createFakekHelper(client);
const widget = algoliaPlacesWidget({ defaultPosition: [2, 2] });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there probably should be a test without defaultPosition too

@@ -10,34 +10,68 @@ class AlgoliaPlacesWidget {
}

this.placesOptions = placesOptions;
this.placesAutocomplete = places(this.placesOptions);

this.state = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not completely sure, but this could be called this.uiState

Haroenv
Haroenv previously approved these changes Aug 3, 2018
Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't leave any more comments and it's working, so LGTM!

(uiState.places.position === this.state.position &&
uiState.places.query === this.state.query)
) {
this.placesAutocomplete.setVal('');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition is when uistate.places is not define or position and query are the same which doesn't mean that the query is empty.

@raphi
Copy link
Contributor

raphi commented Aug 8, 2018

@bobylito @Haroenv if all is fine with you, can you approve the PR? Thx

Copy link
Contributor

@bobylito bobylito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good and congratulations on the implementation of the instantsearch routing for this widget @JonathanMontane :)

@bobylito
Copy link
Contributor

bobylito commented Aug 8, 2018

@bobylito @Haroenv if all is fine with you, can you approve the PR? Thx

Sorry :)

@JonathanMontane JonathanMontane merged commit 05a8cdb into master Aug 8, 2018
@raphi raphi deleted the feat/is-routing branch August 8, 2018 13:56
.setQueryParameter('aroundLatLng', this.defaultPosition);
.setQueryParameter(
'aroundLatLng',
this.state.position || this.defaultPosition
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this.state.position will ever be defined in that case

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be defined because of the init of the routing which happens before the init of the widget.

},
} = opts;

this.state.position = `${lat},${lng}`;
this.state.query = value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than duplicate the source of truth for the values why doesn't read them directly from the helper & placesAutocomplete?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the query you're looking for. It's actually the value for searching for values in places, which is nowhere in the main search state.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but since you have access to the Places instance you can have access to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although you can simplify the internal state by removing the position information, you cannot make it completely disappear, as the getVal method from the placesAutocomplete instance will yield stale data.

the placesAutocomplete.on('change', () => {}) is triggered before the update of the value of the input, and so is getWidgetState. So placesAutocomplete.getVal will yield the previous input value, instead of the new one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks for the update!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants